Introduce PagedVector class #13808
Introduce PagedVector class #13808
Conversation
|
Starting build on |
Test Results 9 files 9 suites 1d 20h 22m 36s ⏱️ Results for commit b99836b. ♻️ This comment has been updated with latest results. |
|
@ktf, this is a backport of llvm/llvm-project#66430 but does it include llvm/llvm-project#67958? |
|
@ktf this fails quite horribly in CI: @Axel-Naumann I'm not convinced this should be backported to 6.28, this has the potential to break a lot of things... |
|
With the original PR accepted upstream and an experiment requesting this we need good reasons (such as "fails quite horribly in CI" :-) ) to refuse the backport. I think we want to work with @ktf to make this PR work... @ktf the problem is that another patch release is imminent and this PR seems not yet ready for prime time. It will have to miss this train but take the next one. |
|
Looks like we fail with |
Yes, that's exactly what I posted 45 minutes ago...
Yes, that's what I'm saying. And IMHO that "tuning" should not happen after we broke everybody's ROOT in a minor patch release... |
Agreed, let's fix the failures in the master and re-evaluate what should be done next. |
|
I am checking what's going on. I might have simply screwed up the backport. |
Not yet. |
That's fine for me. |
|
Needless to say that the (+ -DLLVM_ENABLE_ASSERTIONS=On) with my setup works fine. Investigating what difference I have in the build environment (I also already tried CXX20...). |
ktf
force-pushed
the
reduce-memory-usage
branch
from
8785b55
to
d6a8bd9
Compare
|
Starting build on |
ktf
force-pushed
the
reduce-memory-usage
branch
from
d6a8bd9
to
27fa1fb
Compare
|
Starting build on |
|
The updated version should be closer to what is currently in llvm upstream... |
What are the differences? Ideally we should apply the exact patch from upstream, it should cleanly go away on a future LLVM upgrade... |
|
Apparently ROOT's llvm-project is not up-to-date. |
https://github.com/root-project/llvm-project/ matches what is currently in |
|
ping... |
|
@ktf ping... |
|
Build failed on windows10/default. Errors:
|
|
I believe you have to rebase, not merge the |
|
Build failed on ROOT-performance-centos8-multicore/soversion. Errors:
|
|
Build failed on ROOT-ubuntu2004/python3. Errors:
|
|
Build failed on mac12arm/cxx20. Errors:
|
hahnjo
force-pushed
the
reduce-memory-usage
branch
from
39c742c
to
7c2fde0
Compare
|
Starting build on |
|
Build failed on windows10/default. Failing tests: |
|
@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=ON |
|
Starting build on |
|
Build failed on windows10/default. Failing tests: |
The goal of the class is to be an (almost) drop in replacement for SmallVector and std::vector when those are presized and filled later, as it happens in SourceManager and ASTReader. By doing so, sparsely accessed PagedVector can profit from reduced memory footprint. Co-authored-by: Jonas Hahnfeld <jonas.hahnfeld@cern.ch>
hahnjo
force-pushed
the
reduce-memory-usage
branch
from
7c2fde0
to
b99836b
Compare
|
Starting build on |
|
Hi @ktf, as discussed yesterday the test failures are related to an issue in our incremental builds where Clad is not rebuilt after changes to the Clang headers. This leads to very weird symptoms because some "stale" functions access memory where they shouldn't and so on. I was hit by this problem already twice and it's tracked in #7977, so one would suppose that I remember by now but evidently I didn't... Apologies for the confusion and the delay it caused in integrating this. I've now synchronized the changes to https://github.com/root-project/llvm-project/releases/tag/ROOT-llvm16-20240116-01, moving the header to Some measurements of this change on my machine: for a simple |
|
Very nice, thank you for your followup. Shall I close this one? |
|
BTW, I think you can move IdentifiersLoaded to be a paged vector as well, although one would need to measure the effect on RSS. |
No, please leave it open: I hijacked your branch and directly pushed the slightly modified commit there. It's currently running through our CI (which is a bit congested at the moment). Once all is green, we can merge 😃
Ok, that's for a follow-up PR I would say. |
|
Hi @ktf, if we were to backport this to a release, which one would ALICE be interested in, 6.30? Or would you be fine keeping the patch locally in your stack for now? |
|
Yes, we would need 6.30, but for now we are fine. |
Ok, I prepared #14411. To be seen if we already include it in 6.30.04 or only in .06 |
|
Perfect, thanks. |
The goal of the class is to be an (almost) drop in replacement for SmallVector and std::vector when those are presized and filled later, as it happens in SourceManager and ASTReader.
By splitting the actual vector in pages of the same size and allocating the pages only when they are needed, using this containers reduces the memory usage by a factor 4 for the cases relevant to the ALICE experiment ROOT / cling usage.